feat(vlm): route VLM GRPO through TQ trainer when data_plane.enabled - #2957
feat(vlm): route VLM GRPO through TQ trainer when data_plane.enabled#2957ZhiyuLi-Nvidia wants to merge 17 commits into
Conversation
|
/ok to test 14e1105 |
14e1105 to
38ef87c
Compare
88aef2c to
9977c05
Compare
|
/ok to test 9977c05 |
9977c05 to
6afd638
Compare
|
/ok to test 6afd638 |
6afd638 to
456b985
Compare
|
/ok to test 456b985 |
| from nemo_rl.algorithms.grpo_sync import grpo_train_sync | ||
|
|
||
| print("🚀 Running synchronous VLM GRPO training (TransferQueue)") | ||
| return grpo_train_sync |
There was a problem hiding this comment.
do we have a plan to update the file name? the current naming is quite confusing
grpo_train_sync -> TQ path while grpo_train -> legacy path
There was a problem hiding this comment.
Yeap, I'd expect this issue would be fixed once legacy path is retired.
|
|
||
| # Packed per-sample: jagged; ``PackedTensor`` in-memory, wire form is | ||
| # ``torch.nested`` parent + ``<key>__lengths`` companion. | ||
| PACKED_MULTIMODAL_FIELDS = frozenset( |
There was a problem hiding this comment.
Suggested by agents - Nemotron-Omni needs imgs_sizes here
There was a problem hiding this comment.
Added. Also added num_frames since processors.py creates it right next to imgs_sizes and they're coupled — would have failed on the next field otherwise.
There was a problem hiding this comment.
can you add a note/TODO to refactor this later into a single source of truth (i.e. ProcessorInterface)
There was a problem hiding this comment.
nemotron-omni now supported.
Also added TODO comment.
| # optional routed_experts under R3 replay. | ||
| present_multimodal = _LP_MULTIMODAL_FIELDS & set(meta.fields or ()) | ||
| lp_fields = fields_with_optional_routed_experts( | ||
| [*LP_SEED_FIELDS, *present_multimodal], |
There was a problem hiding this comment.
These image fields are included for logprobs, but train_from_meta() later fetches only DP_TRAIN_FIELDS, which has no image fields. So logprobs see the image, while the GRPO training update does not.
There was a problem hiding this comment.
Confirmed, fixed. Train now fetches the same multimodal fields as logprob. train/probs_ratio_max and train/probs_ratio_min are normal now.
Addresses PR #2957 review comments. 1. train_from_meta / train_microbatches_from_meta fetched only the static text-only DP_TRAIN_FIELDS, so a VLM training forward ran image-blind while prev/ref logprobs were computed *with* images. Both now ship DP_TRAIN_FIELDS + _present_multimodal_fields(meta), the same per-batch add-on the logprob dispatch uses (renamed _LP_MULTIMODAL_FIELDS -> _WIRE_MULTIMODAL_FIELDS since it is no longer logprob-only). The Qwen3.5-35B-A3B geo3k run shows the fingerprint: train/ probs_ratio_max 216-2661 and probs_ratio_min 0.0 on every step, with probs_ratio_clamped pinned to the 1.2/0.8 clip bounds, while the non-DP baseline holds probs_ratio_max == probs_ratio_min == 1.0 (the strictly-on-policy invariant for a single inner step). Mean probs_ratio stays ~0.998 because only the image tokens are wrong, which is why the metrics compared in the PR body -- all computed upstream of the training forward -- did not move. 2. BatchedDataDict.truncate_tensors narrowed every >=2-D tensor on dim 1 to the microbatch seqlen. The in-memory PackedTensor form is skipped by torch.is_tensor, but the data-plane wire form is a plain tensor whose dim 1 is patch/image count, so dynamic batching would silently corrupt images (or raise narrow: length > size). Skip PACKED_MULTIMODAL_FIELDS and their __lengths companions; per-token fields stay sequence-aligned and still truncate. 3. Register imgs_sizes (Nemotron-Omni, packed along dim 0 per data/processors.py) in PACKED_MULTIMODAL_FIELDS. Tests: train/logprob multimodal field parity + text-only stays empty (tests/unit/data_plane/test_multimodal_wire_roundtrip.py), and truncate_tensors leaving the wire form intact while truncating mm_token_type_ids (tests/unit/data/test_multimodal_dict.py). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
456b985 to
231b2ad
Compare
|
/ok to test 231b2ad |
|
/ok to test c0ae0bd |
Addresses PR #2957 review comments. 1. train_from_meta / train_microbatches_from_meta fetched only the static text-only DP_TRAIN_FIELDS, so a VLM training forward ran image-blind while prev/ref logprobs were computed *with* images. Both now ship DP_TRAIN_FIELDS + _present_multimodal_fields(meta), the same per-batch add-on the logprob dispatch uses (renamed _LP_MULTIMODAL_FIELDS -> _WIRE_MULTIMODAL_FIELDS since it is no longer logprob-only). The Qwen3.5-35B-A3B geo3k run shows the fingerprint: train/ probs_ratio_max 216-2661 and probs_ratio_min 0.0 on every step, with probs_ratio_clamped pinned to the 1.2/0.8 clip bounds, while the non-DP baseline holds probs_ratio_max == probs_ratio_min == 1.0 (the strictly-on-policy invariant for a single inner step). Mean probs_ratio stays ~0.998 because only the image tokens are wrong, which is why the metrics compared in the PR body -- all computed upstream of the training forward -- did not move. 2. BatchedDataDict.truncate_tensors narrowed every >=2-D tensor on dim 1 to the microbatch seqlen. The in-memory PackedTensor form is skipped by torch.is_tensor, but the data-plane wire form is a plain tensor whose dim 1 is patch/image count, so dynamic batching would silently corrupt images (or raise narrow: length > size). Skip PACKED_MULTIMODAL_FIELDS and their __lengths companions; per-token fields stay sequence-aligned and still truncate. 3. Register imgs_sizes (Nemotron-Omni, packed along dim 0 per data/processors.py) in PACKED_MULTIMODAL_FIELDS. Tests: train/logprob multimodal field parity + text-only stays empty (tests/unit/data_plane/test_multimodal_wire_roundtrip.py), and truncate_tensors leaving the wire form intact while truncating mm_token_type_ids (tests/unit/data/test_multimodal_dict.py). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
c0ae0bd to
d265750
Compare
|
/ok to test d265750 |
|
can this PR be validated on The implementation from #3290 explicitly forbids models like Nano-Omni from working with the DataPlane, see |
|
|
||
| # Packed per-sample: jagged; ``PackedTensor`` in-memory, wire form is | ||
| # ``torch.nested`` parent + ``<key>__lengths`` companion. | ||
| PACKED_MULTIMODAL_FIELDS = frozenset( |
There was a problem hiding this comment.
can you add a note/TODO to refactor this later into a single source of truth (i.e. ProcessorInterface)
| # the padding could fix them. Padding to the *global* batch max | ||
| # (not a per-shard max) is deliberate: every DP rank then sees | ||
| # identical trailing dims for the forward. | ||
| if self.pad_to_max_shape: |
There was a problem hiding this comment.
why not flatten the tensor and unflatten after? this can allow jagged tensors at this stage instead of having multiple points to pad the tensor, and is easier to maintain for future ops for mm tensors. maybe im missing something?
There was a problem hiding this comment.
You were right, and this is what the encoder does at head — closing the loop, since nobody replied here.
to_wire flattens each segment to 1-D and makes each row the 1-D concatenation of its segments, then hands the rows to torch.nested.as_nested_tensor(..., layout=torch.jagged). Rows then differ only in dim 0, so jagged accepts ragged trailing dims and mixed rank, and no padding is written into the bytes that cross the wire. The true shapes travel beside the payload on KVBatchMeta.tags, and from_wire splits each row back up by segment numel. Padding now happens once, in worker memory at use time, inside as_tensor.
There was a problem hiding this comment.
can this PR be validated on nvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-BF16 ?
The implementation from #3290 explicitly forbids models like Nano-Omni from working with the DataPlane, see nemo_rl/data_plane/worker_mixin.py:137 . The check was added because PackedTensor objects could not be passed through the TQ, which is now implemented here.
d265750 to
f67dd38
Compare
Mirror the `run_grpo.py` launcher pattern in `run_vlm_grpo.py` so the VLM entrypoint dispatches to `grpo_train_sync` (TransferQueue) when `data_plane.enabled=true` and otherwise stays on the legacy `grpo_train`. The policy factory is also selected at the launcher level (`TQPolicy` when enabled) so the legacy trainer remains data-plane-agnostic per the architecture invariant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
Mirror `grpo.py`'s `_initial_policy_generation_stale` check that the legacy trainer uses to seed `POLICY_GENERATION_STALE`. The sync trainer hardcoded `POLICY_GENERATION_STALE = True`, forcing a refit at iter 1 even when setup() had already synced weights (`synchronizer.is_stale` is `False`). The redundant refit resets vLLM's CUDA-graph capture and prefix-cache / KV-cache state, so vLLM's step-1 `generation_logprobs` disagree with Megatron's re-scoring on the same tokens by ~ln(40) nats/token (`train/token_mult_prob_error` ~ 40 at step 1, converging by step 3). Loss is unaffected (`probs_ratio = 1` throughout), but the diagnostic metric spike is cosmetic noise and gets masking-thresholded in some configs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
The TQ sync trainer's ``sync_rollout_actor`` silently dropped
PackedTensor multimodal fields (pixel_values, image_grid_thw,
mm_token_type_ids for Qwen2.5-VL / Qwen3-VL, token_type_ids for
Gemma3) because the write path filtered by
``isinstance(v, torch.Tensor)`` — ``PackedTensor`` is not a
``torch.Tensor`` subclass. Consequence: trainer's ``prev_logprobs``
were computed without image embeddings while vLLM's
``generation_logprobs`` used them, producing on Qwen3.5-A3B-Base +
geometry3k:
* train/token_mult_prob_error avg ~176 (should be ~1.02)
* train/sampling_importance_ratio ~0.982 uniform (should be ~1.000)
* train/policy_kl_error avg ~175 (should be ~0.02)
Loss was on-policy (probs_ratio = 1) so training didn't crash, but
the diagnostic metrics flagged the mismatch clearly.
Fix threads multimodal fields end-to-end via a binary wire dispatch
(torch.Tensor | np.ndarray[object]) with these pieces:
* nemo_rl/data_plane/field_registry.py — new FieldSpec (alignment /
container / stages) collapses five sprinkled schema constants
(DP_TRAIN_FIELDS, LP_SEED_FIELDS, TOKEN_ALIGNED_FIELDS,
ADDITIONAL_OPTIONAL_KEY_TENSORS, LP_STAGE_EXCLUDED) into one
registry. Structural container check catches mis-registrations
loudly at write time.
* nemo_rl/experience/sync_rollout_actor.py — write-side converts each
PackedTensor to torch.nested.NestedTensor (jagged layout) +
companion ``<key>__lengths`` int32 tensor before crossing the wire
boundary. None entries are padded with zero-length tensors to keep
the nested batch dim aligned with lengths (mixed text/image
batches). SmolVLM (dim_to_pack=1) fails loudly with
NotImplementedError — ragged_idx threading is a follow-up.
* nemo_rl/distributed/batched_data_dict.py::get_multimodal_dict —
read-side reconstructs the per-sample PackedTensor by slicing the
materialize-padded rectangular tensor at ``__lengths[i]``.
Defensive assertion on batch-dim alignment between the parent and
companion.
* nemo_rl/data_plane/codec.py — pack_jagged_fields dispatch is binary
(Tensor + ndarray[object]); errors loudly on any other type. No
packed_tensor container category.
* nemo_rl/data_plane/column_io.py::kv_first_write — filter matches
the codec's binary universe (Tensor incl. torch.nested; ndarray[object]).
* nemo_rl/models/policy/tq_policy.py::_logprob_dispatch — fetches
via field_registry.fields_for_stage("logprob" | "ref_lp") instead
of a hardcoded include list. New multimodal keys drop into the
registry as one line each; no per-stage constant updates needed.
Diagnostic sub-timers (rollout / flatten / bulk_assemble /
kv_first_write / finish_gen) emit under timing/rollout/sub_* so the
driver-side ``generation`` timer can be decomposed for perf analysis.
Empirical verification on Qwen3.5-A3B-Base + geometry3k (2N x 8G,
container nemo-rl:nightly-07242026):
* SIR 1.00005 / 1.00002 / 0.99998 (bit-for-bit legacy parity)
* tmpe step 1..3 = 1.019 / 1.020 / 1.057
* mean(3-9) total_step_time: TQ 122.0s vs Legacy 125.3s — **TQ is
3.3s faster at steady state**. sub_kv_first_write dropped from
~2.0s (np.ndarray[object] of PackedTensor wrappers, per-object
pickle) to ~0.64s (native torch.nested storage buffer). Ray RPC
(driver ↔ actor) ~2.8s, TQ put_samples ~0.64s, net TQ tax on
generation ~3.4s, offset by ~5s savings in policy_training from
per-worker TQ fetch. Net TQ throughput ~2.6% faster than legacy.
Known limitations documented in code:
* SmolVLM ``dim_to_pack=1`` support requires ragged_idx handling
(follow-up).
* mm_token_type_ids inclusion in the logprob fetch is what
actually fixes Qwen2.5-VL / Qwen3-VL 3D RoPE positional encoding
for image tokens on the trainer side.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
Adds three tests exercising the VLM multimodal wire path added by the preceding commit: - `test_get_multimodal_dict_full_vlm_wire_roundtrip` — build a realistic batch (pixel_values / image_grid_thw as PackedTensor with a None entry, mm_token_type_ids as per-token rectangular tensor, plus non-multimodal input_ids/token_mask). Encode via `to_nested_wire`, simulate materialize (`to_padded_tensor`), decode via `from_nested_wire`. Assert every packed field's `.as_tensor()` matches pre-wire, per-token fields pass through unchanged, and non-multimodal keys are silently skipped. Covers both `as_tensors=True` and `as_tensors=False` paths. - `test_get_multimodal_dict_missing_companion_asserts` — wire-form parent without its `__lengths` companion must raise with a wire-contract message, not a bare KeyError deep in trainer forward. - `test_get_multimodal_dict_empty_batch_skips_wire_field` — 0-row DP shard: `from_nested_wire` returns None and the read side skips the field (no crash on `PackedTensor.__init__`'s len>0 assert). Also adds a static registry check: the field names used by the tests must be in `PACKED_MULTIMODAL_FIELDS` / `PER_TOKEN_MULTIMODAL_FIELDS` — guards against a future rename that would silently drop the field. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
End-to-end verification through the data-plane ABC contract: build a rollout batch with PackedTensor pixel_values + image_grid_thw + mm_token_type_ids → simulate sync_rollout_actor's write loop → kv_first_write to NoOpDataPlaneClient → read_columns (materialize) → assert get_multimodal_dict on the fetched batch matches the pre-wire .as_tensor() output. Guards the silent-drop regression class this PR was written to fix, plus the pad_to_seqlen exclusion for multimodal fields (asserts pixel_values shape stays [B, max_patches, ...] and doesn't inflate to [B, seqlen, ...]). Complements the two BatchedDataDict-layer defensive tests in tests/unit/data/test_multimodal_dict.py (companion-missing and empty-batch guards). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
Addresses PR #2957 review comments. 1. train_from_meta / train_microbatches_from_meta fetched only the static text-only DP_TRAIN_FIELDS, so a VLM training forward ran image-blind while prev/ref logprobs were computed *with* images. Both now ship DP_TRAIN_FIELDS + _present_multimodal_fields(meta), the same per-batch add-on the logprob dispatch uses (renamed _LP_MULTIMODAL_FIELDS -> _WIRE_MULTIMODAL_FIELDS since it is no longer logprob-only). The Qwen3.5-35B-A3B geo3k run shows the fingerprint: train/ probs_ratio_max 216-2661 and probs_ratio_min 0.0 on every step, with probs_ratio_clamped pinned to the 1.2/0.8 clip bounds, while the non-DP baseline holds probs_ratio_max == probs_ratio_min == 1.0 (the strictly-on-policy invariant for a single inner step). Mean probs_ratio stays ~0.998 because only the image tokens are wrong, which is why the metrics compared in the PR body -- all computed upstream of the training forward -- did not move. 2. BatchedDataDict.truncate_tensors narrowed every >=2-D tensor on dim 1 to the microbatch seqlen. The in-memory PackedTensor form is skipped by torch.is_tensor, but the data-plane wire form is a plain tensor whose dim 1 is patch/image count, so dynamic batching would silently corrupt images (or raise narrow: length > size). Skip PACKED_MULTIMODAL_FIELDS and their __lengths companions; per-token fields stay sequence-aligned and still truncate. 3. Register imgs_sizes (Nemotron-Omni, packed along dim 0 per data/processors.py) in PACKED_MULTIMODAL_FIELDS. Tests: train/logprob multimodal field parity + text-only stays empty (tests/unit/data_plane/test_multimodal_wire_roundtrip.py), and truncate_tensors leaving the wire form intact while truncating mm_token_type_ids (tests/unit/data/test_multimodal_dict.py). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
run_vlm_grpo passes processor= to whichever trainer _select_trainer
returns, but grpo_train_sync did not accept it, so every VLM run with
data_plane.enabled=true died after full model load with:
TypeError: grpo_train_sync() got an unexpected keyword argument
'processor'
grpo_train uses processor for exactly one thing:
attach_initial_nemo_gym_image_payloads, gated on
grpo.deduplicate_multimodal_data. That flag is already rejected for
data_plane.enabled=true by _validate_multimodal_dedup_capability, so the
sync trainer can never need the object -- accept it for signature parity
and assert the invariant instead of ignoring the argument, so relaxing
that upstream guard fails loudly rather than silently dropping image
payloads.
Adds test_sync_trainer_is_call_compatible_with_legacy_trainer, which
diffs the two signatures. The e2e that surfaced this costs two nodes and
~12 minutes of setup before it fails; the signature check is instant.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
The DTensor/AutoModel backend has its own sequence-dim pre-flight,
check_sequence_dim, which is the analog of megatron's
get_and_validate_seqlen. Only the megatron one was taught to skip
multimodal fields, so a TQ VLM run on the automodel backend died at
step 1:
AssertionError: Dim 1 must be the sequence dim, expected dim 1=2432
but got shape torch.Size([32, 1, 3])
[32, 1, 3] is image_grid_thw (batch, num_images, t/h/w). Packed
multimodal fields are never sequence-aligned -- dim 1 is
num_images/num_patches -- and their <key>__lengths wire companions are
1-D. In-memory these ride as PackedTensor and are skipped by
torch.is_tensor, but the data-plane wire form is a plain tensor.
The skip goes inside check_sequence_dim rather than onto its existing
skip_keys parameter: all seven call sites need it, and none of them
should have to know the wire format.
Also collapses three byte-identical inline copies of the same check in
the v1 DTensor worker into calls to check_sequence_dim. v1 is the
default worker (dtensor_cfg._v2 defaults to false), so it had the same
latent bug; sharing the helper keeps the skip in one place.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
Multi-agent review of the VLM/TQ path. Each item below is either
reproduced on hardware or verified against source; severities are
post-adversarial-review.
1. mooncake_cpu was dead on arrival for any VLM run. The
`<key>__lengths` companions minted by `PackedTensor.to_nested_wire`
are dense int32[B], and `_promote_1d_leaves` rejects any dense 1-D
field not declared in PROMOTE_1D_FIELDS. Reproduced on 1 node:
ray::SyncRolloutActor.rollout_to_tq()
ValueError: Mooncake field 'pixel_values__lengths' is a dense 1D
tensor but is not declared in data_plane.schema.PROMOTE_1D_FIELDS
Derived the companion names from PACKED_MULTIMODAL_FIELDS rather
than hand-listing them, so a new packed modality is covered
automatically. Fixes the read side too -- `_from_wire` squeezes only
declared fields. Verified: same job now runs both steps clean.
Missed because every e2e used backend=simple, where the transform
is a no-op.
2. The rollout write dropped `pixel_dtype`, which every legacy analog
passes (grpo.py:2106/:3195, grpo_sync.py:644) and no worker
re-applies. pixel_values are deliberately fp32 out of the processor
(see data/processors.py), so they crossed the wire fp32 where legacy
shipped bf16 -- 2x the largest column. Verified by inspection of all
call sites, not measured.
3. `to_nested_wire` concatenated a logical row's segments before
applying pad_to_max_shape, inverting `as_tensor`'s order; a
multi-segment dedup row with differing trailing dims raised in
torch.cat before the padding could run. Now pads segments first.
Latent today (dedup is rejected under data_plane) -- trap removal.
4. `from_nested_wire` turned zero-length rows into empty tensors rather
than None, so an image-free DP shard produced pixel_values of shape
(0, ...) where legacy gives None, and logical_segment_counts_by_row
reported 1 instead of 0. Also added a companion/row length check --
a short companion would silently misalign images against samples.
5. fp8 QKV calibration filtered on DP_CALIB_INPUT_FIELDS, which names a
`multi_modal_inputs` column that is never written, so a VLM run
calibrated image-blind. Newly reachable: run_vlm_grpo only started
routing to grpo_train_sync in this PR.
6. Docs/typing: the `_logprob_dispatch` docstring stated the opposite of
what the code does (a reader following it would restore the
image-blind bug); the registry comment claimed to be the "single
source of truth" when data/processors.py performs the actual
classification; a comment cited a test path that does not exist;
TOKEN_ALIGNED_FIELDS hand-duplicated PER_TOKEN_MULTIMODAL_FIELDS;
`encode_multimodal_for_wire` was unannotated in a pyrefly-checked
file.
Not addressed here, both needing a product decision: SmolVLM cannot run
with data_plane.enabled (get_dim_to_pack_along returns 1, which
to_nested_wire rejects) and wants a setup()-time gate; and VLM+TQ has no
nightly coverage because tests/test_suites/llm/common-tq.env gates on
^(grpo|dapo|prorlv2)-, which no vlm_grpo-* recipe can match.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
Closes the coverage gaps the review found. 218 passed on GPU (tests/unit/data_plane/ + unit/data/test_multimodal_dict.py). Encoder / wire boundary (test_multimodal_dict.py): - First direct coverage of `encode_multimodal_for_wire`: parent + __lengths emission, per-token passthrough, all-empty skip, unregistered-field KeyError, wrong-type guards. - `to_nested_wire` guard rails: dim_to_pack != 0 -> NotImplementedError, all-None -> (None, None), pad_to_max_shape rank mismatch -> ValueError. - Regression for the pad-before-concat fix: a dedup row spanning 2x4 and 4x2 segments round-trips instead of raising in torch.cat. - Regressions for the companion length check and for empty rows matching legacy None semantics. - truncate_tensors leaves wire-form multimodal intact while still truncating per-token maps. Dispatch (test_multimodal_wire_roundtrip.py): - The roundtrip helper now calls the production `encode_multimodal_for_wire` instead of reimplementing its branches -- the reimplementation was blind by construction to exactly the drift its docstring claimed to catch. - ref_lp and the SC `train_microbatches_from_meta` path each carry their own copy of the multimodal add-on and had none; the SC path could have regressed image-blind while the sync path stayed green. - The train/logprob parity assertion now compares against the full registry; the previous third assertion was implied by the two above it. - materialize's pad_to_seqlen exclusion was never actually exercised -- pad_to_seqlen comes from meta.extra_info, which the test never stamped, so the guard against the ~40x blow-up was untested despite the module docstring claiming otherwise. - `_stub_tq_policy` used `__del__ = None`, which does not suppress the destructor: CPython installs tp_finalize whenever __del__ is in the class dict, then calls None() -> TypeError, surfacing as PytestUnraisableExceptionWarning charged to whichever test is at GC. Architecture invariants (test_architecture_invariants.py): - run_vlm_grpo's `_select_trainer` copy is now pinned; only run_grpo's was, and the VLM launcher is the one that shipped the processor= TypeError. - The signature-compat check now binds the launcher's actual call shape rather than demanding full parameter parity, which would have forced every future grpo_train parameter into grpo_train_sync as dead weight. Seq-dim skip (test_automodel_train.py): - check_sequence_dim's multimodal skip, plus a negative case proving per-token fields are still validated. The matching get_and_validate_seqlen tests are not included: the container ships neither megatron.bridge nor transformer_engine, so they could not be verified here (the pre-existing TestGetAndValidateSeqlen cannot run in it either). They want the mcore CI shard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
Two type errors from the Lint check, both introduced by the preceding review-fix commit: - `row_segments` typed as list[list[Tensor]] but built by a comprehension whose `is not None` filter does not narrow Optional[Tensor], so it inferred list[list[Tensor | None]]. Rebuilt with an explicit loop. - `encode_multimodal_for_wire` yielded the `__lengths` companion while only guarding `nested is None`; `to_nested_wire` returns Optional for both, so the yield did not match the newly declared Iterator[tuple[str, Tensor]]. Guard both. The second is a direct consequence of annotating the function in that commit -- the annotation is what exposed the looseness. ruff was clean throughout, which is why this only surfaced in CI. Verified with pyrefly in the nightly container (errors shown: 0); the repo venv here cannot install dev deps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
…anions TQ already tracks per-row shape and RL was overriding it in two places. Both overrides existed to undo something RL itself did, so remove the cause rather than the compensation. 1. Multimodal `<key>__lengths` companions `codec.materialize` ran `to_padded_tensor` on every nested leaf, rectangularizing packed multimodal fields to `[B, max_rows, ...]` and destroying the row boundaries. The `<key>__lengths` companion field existed solely to recover them. TQ stores one entry per row (`storage/managers/base.py::_generate_values` unbinds nested fields), so the value it hands back already carries the true per-row shapes. Multimodal fields now skip `to_padded_tensor` and stay nested; `PackedTensor.from_wire` unbinds instead of slicing by companion length. That removes a wire field per packed modality, the derived `PROMOTE_1D_FIELDS` union, the `_WIRE_MULTIMODAL_FIELDS` companion tier, and the `.endswith(LENGTHS_SUFFIX)` skips in both seq-dim validators and `truncate_tensors`. `pad_to_max_shape` is deliberately kept. It is not a transport artifact: `as_tensor` pads to the same batch max, and a deduplicated logical row can span segments with differing trailing dims that `torch.cat` cannot join otherwise. 2. `PROMOTE_1D_FIELDS` `transfer_queue.metadata.extract_field_schema` rebinds a *local* for 1-D inputs (`value = value.unsqueeze(-1)`) and derives the sample shape from it, while `_generate_values` iterates the *original* `(N,)` tensor into N 0-d rows. The schema claims `(1,)`; storage holds `()`. Only the KV path notices, because it is the only one that reconstructs from the schema -- `SimpleStorage` fetches stored objects and never consults it. RL compensated by reshaping the payload to match the wrong schema. `_patch_scalar_field_schema` fixes the schema instead, so the reported shape matches the stored rows and the column stacks back to a dense `(N,)`. It rebinds in all three importing modules (both storage managers bind the name at import time) and is self-verifying: it probes `KVStorageManager._generate_values` and refuses to install if a TQ revision ever starts storing 1-D fields as `(1,)` rows. This covers every dense 1-D field rather than a declared allowlist, so `PROMOTE_1D_FIELDS`, `_promote_1d_leaves` and `_from_wire`'s squeeze branch are all deleted. Tested: 255 passed / 11 skipped across tests/unit/data_plane/ plus test_multimodal_dict.py; 398 passed on test_batched_data_dict.py, tests/unit/data/ and test_automodel_train.py. The 11 skips are the mooncake_cpu fixtures (no RDMA on the test host), so the KV path the scalar-schema patch targets is NOT yet covered by a run -- that and Nemotron-Omni's pad_to_max_shape path remain unverified end to end. Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects kept VLM GRPO from running on the TransferQueue data plane, each reproduced before fixing: * ``BatchedDataDict.slice`` indexed a raw ``torch.nested`` value on its ragged dim. A nested tensor satisfies ``isinstance(v, Tensor)`` and has a valid ``shape[0]``, so it cleared both guards and failed only at the indexing. ``codec.materialize`` now reassembles packed multimodal fields into ``PackedTensor`` at the decode boundary, restoring the invariant that no raw nested value reaches a consumer. * ``_from_wire`` densified any nested field whose rows happened to share a shape, which for a packed field is a data-dependent accident (every sample carrying one image) that discards row boundaries. * ``_validate_multimodal_dedup_capability`` rejected every ``data_plane.enabled=true`` config, blocking all six Nemotron-Omni recipes. The gap it guards is NeMo-Gym specific -- ``grpo_train_sync`` does not call ``attach_initial_nemo_gym_image_payloads``, itself gated on ``should_use_nemo_gym`` -- so the check now names that combination. ``to_wire`` additionally flattens each segment to 1-D. Rows then vary only in dim 0, so ``torch.jagged`` accepts ragged trailing dims and mixed rank alike; the batch-max padding it used to materialize is gone from the wire and from TQ storage, and TQ never falls back to the deprecated strided layout. Padding moves to ``as_tensor``, where a rectangle is actually required -- it is a model input constraint, not a transport one. The shapes flattening removes travel on ``KVBatchMeta.tags``, the transport's per-sample channel, projected with the rows by ``subset``/``slice``/``concat``. ``shard_meta_for_dp`` did not propagate tags and would have dropped them on every per-rank fetch. Nothing in ``nemo_rl/data_plane`` interprets them. ``_global_pad_shape`` pins the batch-wide pad target so every DP rank sees identical trailing dims; a rank-local max let the logprob and training passes encode the same media at different widths. It propagates through all nine ``PackedTensor`` constructors -- ``concat`` dropping it silently reverted ``shard_by_batch_size`` to a per-shard max. Tested: * Qwen3.5-35B-A3B geo3k 2n8g, 20/20 steps. This recipe trains one inner step per rollout, so ``probs_ratio`` is an identity: measured exactly 1.000000 on every step, across three successive wire formats. * Nemotron-Omni-30B-A3B clevr 1n8g, 10/10 steps. ``token_mult_prob_error`` 1.0138-1.0153 against 1.0138-1.0155 for the same recipe with ``data_plane.enabled=false``. * 245 passed / 11 skipped on tests/unit/data_plane and test_multimodal_dict. Nightly gates match what each recipe can actually assert: Qwen3.5 gates ``probs_ratio`` exactly; Nemotron-Omni trains 16 inner steps, where that metric measures policy drift and its max ranges 5.85-29.21 run to run on identical code (the non-data-plane path included), so it gates ``token_mult_prob_error`` instead. Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
f67dd38 to
8d5e0fd
Compare
|
/ok to test 8d5e0fd |
Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
bde7809 to
530eca4
Compare
Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
The pad width is scratch the model discards -- mcore crops it via imgs_sizes before patchification, and the AutoModel path rejects mixed-resolution batches outright -- so no consumer needs shards to agree on it. Drop the transported max from the row tags and the _global_pad_shape plumbing that carried it; as_tensor now computes the max over the rows it holds. Row tags keep shapes (payload geometry, unrecoverable after to_wire flattens) and pad (the field's policy flag), so the data plane no longer carries a pad target it cannot interpret. Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
Signed-off-by: Cory Ye <cye@nvidia.com>
| # image tensors a Gym dataset omits from ``extra_env_info``. That helper is | ||
| # itself gated on ``should_use_nemo_gym``, so non-Gym recipes never needed | ||
| # it and are unaffected. | ||
| if (master_config.data_plane or {}).get("enabled", False) and ( |
There was a problem hiding this comment.
nemo_rl/algorithms/grpo.py:473
The premise of this relaxation does not hold: on the TQ trainer, deduplication never happens at all, so the flag becomes a silent no-op rather than a supported combination.
enable_deduplication() has exactly one caller in the tree — batched_data_dict.py:72, inside _prepare_multimodal_sharing, which repeat_interleave reaches only when share_immutable_media=True. The legacy trainer passes it:
# grpo.py:3069-3073
batch.repeat_interleave(
master_config.grpo.num_generations_per_prompt,
share_immutable_media=(master_config.grpo.deduplicate_multimodal_data),
)grpo_sync.py:603 does not — it is a bare batch.repeat_interleave(num_generations_per_prompt), and this PR does not change it. So provenance is never assigned, _row_offsets stays None, and the deepcopy at batched_data_dict.py:969 runs with an empty memo — G independent copies of every image in driver RAM and G on the wire.
This is live in a recipe this PR adds: vlm_grpo-nemotron-omni-30ba3b-clevr-1n8g-automodel-ep8.v1-tq_simple.yaml sets data_plane.enabled: true and inherits deduplicate_multimodal_data: true from its base (line 3), with G=16. A user setting a memory-saving flag gets zero saving and no warning. The run went green because clevr images are small.
Worth knowing for whichever fix you pick: even with sharing enabled, to_wire emits one row per logical row, and torch.nested.as_nested_tensor(..., layout=torch.jagged) routes to jagged_from_list, which does values = torch.cat(tensors, dim=ragged_idx - 1) — so the wire payload is O(G × images) regardless. Carrying the sharing across the wire would mean emitting each physical segment once and putting the CSR map in the tags channel multimodal_row_tags already uses; from_wire already accepts _row_offsets / _segment_indices.
Three options, in order of effort: pass share_immutable_media=master_config.grpo.deduplicate_multimodal_data at grpo_sync.py:603 and accept that the wire still expands; restore the guard for the TQ path and drop deduplicate_multimodal_data: true from the new clevr recipe; or carry the provenance. Silently accepting a no-op flag is the one option worth avoiding.
There was a problem hiding this comment.
Rohit: Since your PR already added wiring for deduped PackedTensors, it is worthwhile to not block this path at all. With the current guard, the PR blocks NemoGym paths where dedup is not used.
I think the better raise condition for this is to check if master_config.grpo.deduplicate_multimodal_data is set to True instead of checking this in NeMo-Gym. Otherwise this blocks sync GRPO with NemoGym (even text only paths) for no reason. We can validate the dedup path later.
@terrykong might need your opinion on this.
There was a problem hiding this comment.
IIUC the question is grop.py (legacy) passes share_immutable_media=(master_config.grpo.deduplicate_multimodal_data), into the repeat_interleave but grpo_sync.py doesn't. seems like we should have consistency, so I would say we should match
| # frame count. Coupled with pixel_values (see | ||
| # ``batched_data_dict._COUPLED_MULTIMODAL_KEYS``); both pack on dim 0. | ||
| "imgs_sizes", | ||
| "num_frames", |
There was a problem hiding this comment.
nemo_rl/data/multimodal_utils.py:125
This registry is a static allowlist checked against a key set the processor decides at runtime, and it is short by at least two names for a model family already in examples/configs/recipes/vlm/.
get_multimodal_keys_from_processor unions processor.model_input_names and subtracts the tokenizer's. At the pinned transformers==5.12.1 (uv.lock), Qwen2_5OmniProcessor.model_input_names explicitly appends two names that are in neither registry:
# transformers v5.12.1 processing_qwen2_5_omni.py:363-372
return list(dict.fromkeys(
tokenizer_input_names + feature_extractor_input_names
+ image_processor_input_names + video_processor_input_names
+ ["feature_attention_mask"]
+ ["video_second_per_grid"]
))processors.py:639 wraps every key present in the processed message in a PackedTensor, get_multimodal_dict emits any PackedTensor key with no registry check, and encode_multimodal_for_wire raises KeyError: unregistered multimodal field 'feature_attention_mask' — inside the Ray rollout actor, after model load and a full generation pass. Reachable for vlm_grpo-qwen2.5-omni-{7b-audiomcq,3b-avqa,7b-intent} and vlm_grpo-qwen3-omni-30ba3b-audiomcq as soon as someone sets data_plane.enabled=true; no TQ wrapper exists for them, so CI will not catch it.
Also: second_per_grid_ts in this set looks like the transformers 4.x spelling of the same field — worth checking whether it is now dead.
Two asks. Register feature_attention_mask and video_second_per_grid. And since the key set is processor-derived and open-ended, add a setup-time check next to _validate_multimodal_dedup_capability that intersects get_multimodal_keys_from_processor(processor) against both registries when data_plane.enabled and raises naming the processor and the offending keys — fail-loud is the right call, fail-loud after a generation pass is not.
Related: extract_multimodal_model_inputs force-appends pixel_values_flat and image_num_patches (:1146-1155) and wraps them the same way. No config in this repo currently produces them, so that path is latent — but the setup-time check would cover it too.
There was a problem hiding this comment.
Rohit: You can add the feature_attention_mask and video_second_per_grid features.
And since the key set is processor-derived and open-ended, add a setup-time check next to _validate_multimodal_dedup_capability that intersects get_multimodal_keys_from_processor(processor) against both registries when data_plane.enabled and raises naming the processor and the offending keys — fail-loud is the right call, fail-loud after a generation pass is not.
This motivates the need for a single source of truth with the ProcessorAdapter. No changes required at this point, but important to note.
| grpo_train( | ||
| # ``_select_trainer`` prints which sync trainer it picked. | ||
| trainer = _select_trainer(master_config) | ||
| trainer( |
There was a problem hiding this comment.
The sibling launcher wraps this call in with checkpointer:, and its comment says why:
# run_grpo.py:241-245
# grpo_train_sync defers checkpoint finalization to the checkpointer's
# background threads; the context manager guarantees they are flushed on
# exit. (grpo_train also flushes internally; shutdown() is idempotent.)
with checkpointer:
trainer((
Lines 241 to 245 in 4bcb483
The omission was harmless before this PR, because this launcher only ever called grpo_train, which flushes internally. Now that _select_trainer can return grpo_train_sync, the exception path is uncovered: that trainer calls checkpointer.shutdown() on its normal exits only, so a crash or interrupt after begin_finalization can kill the daemon thread before tmp_step_N is renamed, losing the last checkpoint. CheckpointManager.__enter__ / __exit__ already exist. Reachable on both new recipes.
| trainer( | |
| trainer = _select_trainer(master_config) | |
| with checkpointer: | |
| trainer( |
(the call's arguments need one more level of indentation)
| # Per-row shapes the flattening removes from the payload. ``tags`` | ||
| # is the transport's per-sample channel and is projected with the | ||
| # rows, so no consumer re-keys them. | ||
| tags=multimodal_row_tags(multimodal, len(sample_ids)), |
There was a problem hiding this comment.
nemo_rl/experience/sync_rollout_actor.py:425
to_wire() runs twice per rollout step on the same objects: once inside encode_multimodal_for_wire at line 334, and again inside multimodal_row_tags here, which keeps only shapes and throws the nested value away.
The second pass is not free. torch.nested.as_nested_tensor(..., layout=torch.jagged) routes to jagged_from_list, which does values = torch.cat(tensors, dim=ragged_idx - 1) — so the largest column in the batch is fully copied twice per step and one copy is discarded immediately.
Cheapest fix: give PackedTensor a shapes-only path (the row_segments loop without the as_nested_tensor call) and have multimodal_row_tags use it. Alternatively, have encode_multimodal_for_wire yield the tag rows alongside the wire value so the encode happens once.
Separately, the comment at multimodal_utils.py:889-892 says the single-segment row "stays zero-copy". The reshape(-1) is a view, but the torch.cat above means no row is zero-copy end to end — worth rewording so nobody optimizes against it.
| layout=layout, | ||
| pad_value_dict=pad_value_dict, | ||
| pad_to_seqlen=pad_to_seqlen, | ||
| tags=meta.tags, |
There was a problem hiding this comment.
nemo_rl/data_plane/worker_mixin.py:250
Latent, but worth a note before someone runs a VLM over TQ with TP>1. materialize runs leader-only here and now returns a reassembled PackedTensor for pixel_values. The descriptor built a few lines up gives the tensor path only to isinstance(v, torch.Tensor) (:78-84); everything else becomes ("raw", v) and rides broadcast_object_list. So on the leader-broadcast path the whole pixel column is pickled into the object list and broadcast as a byte tensor on the NCCL device — the opposite of what the function's own docstring says it exists to avoid.
Not reachable in anything this PR ships: the branch needs replica_group.size() > 1, _get_replica_group is the flattened (cp, tp) mesh, and both new recipes inherit dtensor_cfg.tensor_parallel_size: 1 / context_parallel_size: 1 (the tensor_parallel_size: 8 in those YAMLs is under generation.vllm_cfg). And it is new cost rather than a regression — pixel_values never reached the TQ path before.
Suggested: either extend the descriptor with a PackedTensor kind that ships the per-segment dtype/shape list in the object phase and broadcasts the segments as tensors, or raise in the else branch for PackedTensor so the cost cannot be paid silently. A leader-broadcast test with a multimodal field would pin whichever you pick — tests/unit/data_plane/test_leader_broadcast.py is text-only today.
| ) | ||
|
|
||
|
|
||
| def test_to_wire_pads_segments_before_concat_under_dedup(): |
There was a problem hiding this comment.
tests/unit/data/test_multimodal_dict.py:698
The name says the opposite of what the test asserts: the body checks [t.numel() for t in nested.unbind()] == [48], i.e. that no padded bytes cross the wire, and the docstring says so too. Left over from the design 4bcb483 removed.
| def test_to_wire_pads_segments_before_concat_under_dedup(): | |
| def test_to_wire_does_not_pad_segments_before_concat_under_dedup(): |
One more stale test doc, same cause: tests/unit/data_plane/test_arbitrary_shape_roundtrip.py's module docstring says "B and C are what PackedTensor.to_wire cannot express … C is rejected outright by its if len(ranks) != 1: raise ValueError". At HEAD to_wire flattens to 1-D and does carry mixed rank — test_to_wire_carries_mixed_rank_rows asserts exactly that, and the rank check now lives in global_pad_shape returning None rather than raising. A reader will take the wrong model of the encoder from it.
| row_offsets.append(len(segments_flat)) | ||
| return cls( | ||
| segments_flat, # type: ignore[arg-type] | ||
| dim_to_pack=0, |
There was a problem hiding this comment.
im not a fan of the hardcoded dim_to_pack value here since the point of the PackedTensor was to allow arbitrary packing dims. Needs more thorough treatment later. No action needed now.
| ) | ||
|
|
||
|
|
||
| def _promote_1d_leaves(td: TensorDict) -> TensorDict: |
There was a problem hiding this comment.
I'm not super sure if this change is required for this PR. The agent recommends me to drop the two TQ monkeypatches and restore _promote_1d_leaves
The old fix was a writer / reader pair, Mooncake-only:
- put: unsqueeze declared fields (N,) → (N, 1)
- get: squeeze those fields back
- allowlist: PROMOTE_1D_FIELDS in schema.py
- reject any dense 1-D field not on that list
Packed fields are nested jagged tensors, not dense (N,).
There was a problem hiding this comment.
This change does not seem to add multimodal support, so it might be better to either add tests for it (not in scope for this PR) or defer it.
There was a problem hiding this comment.
No action on this thread — answering the scope question, and the ask lands elsewhere.
Don't restore it. This is a replacement, not a deletion:
_patch_scalar_field_schema fixes the
same upstream TQ bug at the schema layer instead of reshaping the payload, and covers every dense
1-D field rather than a hand-kept allowlist. Restoring the old pair would now break it — the
schema reports (), so a writer that unsqueezes to (N, 1) recreates the mismatch from the other
side.
It is in scope, because this PR is what broke the allowlist. Commit a18956dfd records the
crash: ValueError: Mooncake field 'pixel_values__lengths' is a dense 1D tensor but is not declared in data_plane.schema.PROMOTE_1D_FIELDS.
On "add tests or defer" — the tests exist
(test_scalar_field_schema_patch.py,
7 tests, no RDMA needed). What's missing is a run: mooncake is the only backend any of this
affects, and there is no e2e mooncake run at this head. I've asked for one in a separate comment
on the omni wrapper.
| if rows and all(row.shape == rows[0].shape for row in rows[1:]): | ||
| v = torch.stack(rows) | ||
| changed = True | ||
| if field_name in PROMOTE_1D_FIELDS: |
There was a problem hiding this comment.
This chunk of code will need restoring with the _promote_1d_leaves code.
| # train on the media a Gym dataset omits from ``extra_env_info``. | ||
| master_config.data_plane = {"enabled": True} | ||
| with pytest.raises(NotImplementedError, match="data_plane.enabled=false"): | ||
| with patch("nemo_rl.algorithms.grpo.should_use_nemo_gym", return_value=True): |
There was a problem hiding this comment.
test needs to change with the dedup guard on TQ path to test changed functionality
| _WIRE_MULTIMODAL_FIELDS = PER_TOKEN_MULTIMODAL_FIELDS | PACKED_MULTIMODAL_FIELDS | ||
|
|
||
|
|
||
| def _present_multimodal_fields(meta: KVBatchMeta) -> list[str]: |
There was a problem hiding this comment.
This seems to be a pure function and likely belongs to the multimedia_utils instead of grpo_sync.py?
terrykong
left a comment
There was a problem hiding this comment.
Team review of the VLM data-plane path — six agents (RL guidelines, bugs, tests, design, existing threads, adversarial) plus a lead pass, deduplicated against the 33 inline comments already on this PR.
This is good work and it should land. It fixes a measured correctness bug rather than a hypothetical one: the write path filtered on isinstance(v, torch.Tensor) and silently dropped every PackedTensor, so the trainer computed prev_logprobs without image embeddings while vLLM used them — train/token_mult_prob_error around 176 against an expected 1.02. It also nets out simpler where it counts. A competing registry is deleted (ADDITIONAL_OPTIONAL_KEY_TENSORS, two references down to zero), a per-field allowlist workaround is replaced by a fix at the source, a silent-drop comprehension becomes a loud TypeError, and three copy-pasted sequence-dim loops collapse into one helper. Adding a new packed modality now costs a single two-line edit — this PR's own imgs_sizes / num_frames addition proves it.
Nothing below is a reason to hold it. Five inline comments, plus replies on five existing threads.
Please rebase — the PR conflicts with main in two files.
nemo_rl/data_plane/schema.py— trivial, and self-inflicted: the stray blank line this PR adds atschema.py:27(the same one ruff flags) collides with a new block on main. Deleting that line makes the file merge on its own.nemo_rl/models/policy/tq_policy.py— main added atrain_fieldsparameter totrain_microbatches_from_meta. When you resolve it, thread that parameter into the body the waytrain_from_metaalready does:[*train_fields, *_present_multimodal_fields(meta)]. Taking either side verbatim drops one of the two changes.
column_io.py and nightly.txt show as conflicted on GitHub but merge cleanly — main's two new mask fields land inside _TEXT_TOKEN_ALIGNED_FIELDS unaided.
Linter. pre-commit could not run here (the lockfile is linux-only and I am on macOS), so I ran the pinned ruff 0.9.9 directly instead. Six failures, all from this PR — details in the comment on tq_policy.py:53.
Design — no action, recorded because it is worth keeping. We checked whether the two field registries want a seam, a merge, or an extraction, and the answer is none of the three. Adding a packed multimodal field costs one edit site, and all six read-side consumers dispatch off the frozenset by name. Merging PACKED_MULTIMODAL_FIELDS and PER_TOKEN_MULTIMODAL_FIELDS into a single kind-map would scatter a mode check across every consumer to save one declaration — worth not doing. The wire methods do not form an extractable cluster either; we mapped the attribute partition and to_wire shares state with as_tensor, slice and __deepcopy__. Reusing the existing KVBatchMeta.tags channel for per-row shapes beat inventing new plumbing, and keeping reassemble_packed_multimodal in multimodal_utils.py is what lets the codec's dispatch stay binary. The whole wire path is unit-testable on CPU with no Ray, no GPU and no TransferQueue — we exercised it that way.
One nit not worth its own comment: grpo_sync.py:491's error message names only examples/run_grpo.py, so a VLM user who trips it is pointed at the wrong file.
Generated by Claude Code
|
|
||
| # ===== BEGIN CONFIG ===== | ||
| # Mirrors vlm_grpo-nemotron-omni-30ba3b-clevr-1n8g-automodel-ep8.v1.sh (delegated base). | ||
| NUM_NODES=1 |
There was a problem hiding this comment.
tests/test_suites/vlm/vlm_grpo-nemotron-omni-30ba3b-clevr-1n8g-automodel-ep8.v1-tq_simple.sh:6
2 action items.
TL;DR — the PR description documents a mooncake_cpu backend, a cluster.num_nodes: 2 override, and a step-count guard that are not in the diff and were never committed on this branch, so the one change here that only affects mooncake ships with no run behind it.
PR-introduced. I could not run any of this — no GPU or cluster available to me — so the OOM below is quoted from your own description, not something I reproduced.
I checked whether the described code was written and later lost. Searching the branch history for each string over ccbcd4cc..HEAD returns zero commits for num_nodes: 2 under examples/configs/recipes/vlm/, and zero for mooncake_cpu under either examples/configs/recipes/vlm/ or tests/test_suites/vlm/. Both wrapper/YAML pairs landed in a single commit and never contained any of it.
What actually ships:
examples/configs/recipes/vlm/vlm_grpo-nemotron-omni-30ba3b-clevr-1n8g-automodel-ep8.v1-tq_simple.yamlis three lines —defaultsplusdata_plane.enabled: true. Nobackend:, nocluster:.nightly.txt:63says "(backend=simple)" for both entries. That comment is the accurate one.
AI-1
Run one existing -tq_mooncake nightly at this head and post the link — for example grpo-llama3.2-1b-instruct-1n8g-megatron-tq_mooncake.sh.
Replacing _promote_1d_leaves with _patch_scalar_field_schema only changes behaviour on mooncake_cpu. Both runs in the description are simple, and the description labels its mooncake link pre-refactor. Your own commit messages make the point better than I can — a18956dfd:
Missed because every e2e used backend=simple, where the transform is a no-op.
and 79da0856d:
the KV path the scalar-schema patch targets is NOT yet covered by a run
AI-2
Fix the description, or ship the overrides it describes. Three specific claims to reconcile:
- "the wrapper now selects
mooncake_cpuso the two VLM nightlies cover one backend each" — both are-tq_simple, so mooncake has no VLM coverage at all. - The section "Why the omni wrapper sets
cluster.num_nodes: 2". The defaults chain is this YAML tovlm_grpo-nemotron-omni-30ba3b-clevr-1n8g-automodel-ep8.v1.yaml(which sets onlygpus_per_node: 8) tovlm_grpo_3B.yamltogrpo_math_1B.yaml:534,num_nodes: 1— and this wrapper declaresNUM_NODES=1on the anchored line. Withexpert_parallel_size: 8that isdp = 8/8 = 1, the exact configuration your description says "now dies on a 306 MiB allocation inDTensorPolicyWorkerV2.train_presharded()at step 1". By your own analysis the nightly this PR adds cannot finish. - "keeps the driver's status, regenerates
metrics.json... fails the run if it stopped short ofMAX_STEPS". Neither wrapper does any of that — both arebash <base>.shfollowed by onecheck_metrics.pycall. Note the base's own gate is guarded on reaching the step target (vlm_grpo-nemotron-omni-30ba3b-clevr-1n8g-automodel-ep8.v1.sh:33), while this wrapper's gate and its qwen twin are not.
Context — no action. The base recipe is already in nightly at one node (nightly.txt:50), so the 1-node config is not something this PR introduced. The ask covers only the entry this PR adds and the description that explains it.
(No suggestion block: this is a description fix plus a CI run, not a single-region edit.)
| from nemo_rl.data.multimodal_utils import ( | ||
| PACKED_MULTIMODAL_FIELDS, | ||
| reassemble_packed_multimodal, | ||
| ) |
There was a problem hiding this comment.
nemo_rl/data_plane/codec.py:44
1 action item.
TL;DR — this new module-level import makes import nemo_rl.data_plane pull in PIL, requests and 231 transformers submodules, which is exactly what the docstring three lines below says the lazy BatchedDataDict import exists to avoid.
PR-introduced. materialize's docstring at :270-273 states the contract this breaks:
At runtime,
BatchedDataDictis loaded lazily insidematerialize()... to keepimport nemo_rl.data_planecheap for unit tests that don't actually call this function
nemo_rl/data_plane/__init__.py:21 imports materialize, so every importer of the package pays it. multimodal_utils imports requests, PIL, transformers.audio_utils and transformers.video_utils at module scope.
Measured on a CPU-only venv, swapping only this file between the base SHA and head:
codec.py |
import nemo_rl.data_plane |
modules loaded | transformers submodules |
|---|---|---|---|
| base | 1.57s | 2094 | 0 |
| head | 3.40s | 2813 | 231 |
Action: defer both names into materialize, beside the lazy BatchedDataDict import that is already there. That also clears the ruff --select I failure at codec.py:36 (see the lint comment on tq_policy.py:53), because the block disappears.
| from nemo_rl.data.multimodal_utils import ( | |
| PACKED_MULTIMODAL_FIELDS, | |
| reassemble_packed_multimodal, | |
| ) | |
| from nemo_rl.data_plane.schema import Layout |
then inside materialize, next to the existing deferred import:
from nemo_rl.data.multimodal_utils import (
PACKED_MULTIMODAL_FIELDS,
reassemble_packed_multimodal,
)Context — no action. torchvision and torchaudio are not among what gets pulled, so the docstring's specific examples were already loose. The reason it gives is still the one this defeats.
| LP_SEED_FIELDS, | ||
| fields_with_optional_routed_experts, | ||
| ) | ||
| from nemo_rl.distributed.batched_data_dict import BatchedDataDict |
There was a problem hiding this comment.
nemo_rl/models/policy/tq_policy.py:53
1 action item.
TL;DR — the repo's pinned ruff 0.9.9 fails on six things, every one introduced by this PR, so the lint job goes red.
PR-introduced. pre-commit would not run for me (the lockfile is linux-only and I am on macOS), so I ran the pinned version directly — .pre-commit-config.yaml:11 pins rev: v0.9.9, matching pyproject.toml:277.
| where | rule | what |
|---|---|---|
tq_policy.py:53 |
F401 | BatchedDataDict imported and never used — it appears only in the module docstring at :19 |
test_codec_mooncake.py:25 |
F401 | pytest unused after the deleted tests |
codec.py:36 |
I001 | extra blank line added to the import block |
test_codec_mooncake.py:23 |
I001 | |
test_multimodal_wire_roundtrip.py:30 |
I001 | multimodal_row_tags out of order inside the import |
test_codec_mooncake.py |
format | one blank line before test_from_wire_densifies_uniform_nested_rows |
These six are the only ruff failures anywhere in the repo, and each file is clean at the base SHA, so they all come from this PR.
Action: run uv run ruff check --fix . && uv run ruff check --select I --fix . && uv run ruff format .. For this line specifically, delete the import:
| from nemo_rl.distributed.batched_data_dict import BatchedDataDict | |
| from nemo_rl.models.policy.lm_policy import Policy |
Context — no action. I001 is invisible to a plain ruff check, because [tool.ruff.lint] select is only ["D", "F"] — it needs the separate --select I hook at .pre-commit-config.yaml:15. Worth knowing if you were checking locally with a bare ruff check and saw it pass.
| # (logprobs/advantages/masks) and wire-only message | ||
| # log bulk fields are skipped by virtue of not being | ||
| # in DP_CALIB_INPUT_FIELDS. | ||
| # ``DP_CALIB_INPUT_FIELDS`` names a ``multi_modal_inputs`` |
There was a problem hiding this comment.
nemo_rl/algorithms/grpo_sync.py:975
1 action item, low priority.
PR-introduced. This new comment and the comment on the constant it describes now say opposite things.
Here:
DP_CALIB_INPUT_FIELDSnames amulti_modal_inputscolumn that is never actually written
multi_modal_inputscovers VLM extras (pixel values, grid metadata, etc.) when present; it's harmlessly absent for text-only models so the filter skips it on those.
You are right and the older one is wrong. Repo-wide, the only non-comment occurrences of multi_modal_inputs are the tuple entry itself and a test docstring — nothing writes it. Both consumers use it purely as a filter, so a name that is never present is a no-op and dropping it changes no behaviour.
Action: delete the dead entry and the stale sentence in schema.py:94-97, leaving DP_CALIB_INPUT_FIELDS = (INPUT_IDS, INPUT_LENGTHS). The real VLM coverage is the _present_multimodal_fields(meta) you add three lines below this comment.
(No suggestion block: the edit lands in schema.py, which has no diff hunk near those lines.)
Context — no action. The same filter shape sits untouched in the SingleController trainer at single_controller.py:1899-1903 and did not get your _present_multimodal_fields add-on. It is latent — no VLM + SingleController recipe ships, and worker_mixin.py:138 rejects the one model family that would reach it — so this is not an ask, just a pointer for whoever takes VLM to SingleController.
| assert _select_trainer(cfg_sync) is grpo_train_sync | ||
|
|
||
|
|
||
| def test_run_vlm_grpo_dispatches_both_trainers(): |
There was a problem hiding this comment.
tests/unit/data_plane/test_architecture_invariants.py:51
1 action item, low priority.
PR-introduced. Your own docstring on this test makes the argument better than I would:
run_vlm_grpoduplicatesrun_grpo's_select_trainerverbatim, and only therun_grpocopy was pinned — so the VLM dispatch could drift silently. That is not hypothetical: this launcher is the one that shipped theprocessor=TypeError.
The same reasoning applies to a second copied block that did not get a test. run_vlm_grpo.py clones two things from run_grpo.py, and they have to agree — turning the data plane on means picking grpo_train_sync and building a TQPolicy:
| block | run_vlm_grpo.py |
run_grpo.py |
pinned? |
|---|---|---|---|
_select_trainer |
L37-51 | L43-56 | yes, by this test |
| TQPolicy factory | L130-142 | L145-157 | no |
Action: add a sibling test asserting that the VLM launcher builds a TQPolicy when data_plane.enabled is true and passes policy_factory=None when it is false, so both halves of the dispatch are pinned instead of one.
Context — no action. I am not asking you to factor the two launchers into a shared helper. That is a bigger reshape than this PR should carry, and it would move complexity rather than remove it.
| ) | ||
|
|
||
|
|
||
| def _promote_1d_leaves(td: TensorDict) -> TensorDict: |
There was a problem hiding this comment.
No action on this thread — answering the scope question, and the ask lands elsewhere.
Don't restore it. This is a replacement, not a deletion:
_patch_scalar_field_schema fixes the
same upstream TQ bug at the schema layer instead of reshaping the payload, and covers every dense
1-D field rather than a hand-kept allowlist. Restoring the old pair would now break it — the
schema reports (), so a writer that unsqueezes to (N, 1) recreates the mismatch from the other
side.
It is in scope, because this PR is what broke the allowlist. Commit a18956dfd records the
crash: ValueError: Mooncake field 'pixel_values__lengths' is a dense 1D tensor but is not declared in data_plane.schema.PROMOTE_1D_FIELDS.
On "add tests or defer" — the tests exist
(test_scalar_field_schema_patch.py,
7 tests, no RDMA needed). What's missing is a run: mooncake is the only backend any of this
affects, and there is no e2e mooncake run at this head. I've asked for one in a separate comment
on the omni wrapper.
| # the padding could fix them. Padding to the *global* batch max | ||
| # (not a per-shard max) is deliberate: every DP rank then sees | ||
| # identical trailing dims for the forward. | ||
| if self.pad_to_max_shape: |
There was a problem hiding this comment.
You were right, and this is what the encoder does at head — closing the loop, since nobody replied here.
to_wire flattens each segment to 1-D and makes each row the 1-D concatenation of its segments, then hands the rows to torch.nested.as_nested_tensor(..., layout=torch.jagged). Rows then differ only in dim 0, so jagged accepts ragged trailing dims and mixed rank, and no padding is written into the bytes that cross the wire. The true shapes travel beside the payload on KVBatchMeta.tags, and from_wire splits each row back up by segment numel. Padding now happens once, in worker memory at use time, inside as_tensor.
| # image tensors a Gym dataset omits from ``extra_env_info``. That helper is | ||
| # itself gated on ``should_use_nemo_gym``, so non-Gym recipes never needed | ||
| # it and are unaffected. | ||
| if (master_config.data_plane or {}).get("enabled", False) and ( |
There was a problem hiding this comment.
IIUC the question is grop.py (legacy) passes share_immutable_media=(master_config.grpo.deduplicate_multimodal_data), into the repeat_interleave but grpo_sync.py doesn't. seems like we should have consistency, so I would say we should match
Summary
Route VLM GRPO through the TransferQueue data plane when
data_plane.enabled=true, and thread VLM multimodal payloads (pixel_values,image_grid_thw,mm_token_type_ids,imgs_sizes,num_frames) through the TQ wire.examples/run_vlm_grpo.pymirrors therun_grpo.pylauncher pattern: dispatch togrpo_train_syncwhen the data plane is enabled, otherwise stay on the legacygrpo_train.torch.nestedpayload plus per-rowtagscarrying the true segment shapes.PackedTensor.to_wire/from_wireare the only boundary between the domain wrapper and the wire; there are no<key>__lengthscompanion columns and nothing is padded in transit.materialize's pad-to-seqlen skip,truncate_tensors, both per-backend seq-dim validators, the TQ fetch list) dispatch through two registries innemo_rl/data/multimodal_utils.py.data_plane.enabled=false(default).What crosses the wire
The codec dispatches on value type only (
Tensorvs objectndarray); which lane a tensor takes is a registry lookup on the field name.Per-token multimodal maps are text-shaped — one value per token — so they union into
TOKEN_ALIGNED_FIELDSand travel the jagged path withinput_ids. Only the packed modalities needPackedTensor, because their rows are ragged in dimensions unrelated to sequence length. Anything that is neither aTensornor an objectndarrayraisesTypeError, including a rawPackedTensor: it must be converted at the wire boundary insync_rollout_actor.py, which keeps the codec's dispatch binary instead of growing a type ladder.Return path:
Rebuilding
PackedTensorat the decode boundary is what upholds the invariant that no raw nested value reaches a consumer — without itBatchedDataDict.sliceindexes a nested tensor on its ragged dim and fails deep in the call stack.Where padding happens
Padding is a model-input constraint, not a transport one, so it moved to the last possible moment —
as_tensor, on the shard that is about to run the forward.Full path, and who owns what:
Test plan
Both nightly TQ wrappers pass on this branch's head, one per data-plane backend (
simplefor Qwen3.5,mooncake_cpufor Nemotron-Omni). Containernemo-rl:nightly-08102026.squashfs.vlm_grpo-qwen3.5-35ba3b-geo3k-2n8g-automodel-ep16-tq_simple— 20/20 steps, 2N x 8G.Wandb: https://wandb.ai/nvidia/nemorl-dataplane-zhiyul/runs/e2bdvvkb
max(train/probs_ratio_max)min(train/probs_ratio_min)max(train/reward)This recipe trains one inner step per rollout, so the importance ratio is an identity: the training forward runs on the very weights that produced
prev_logprobs. Any deviation means the two passes saw different data — exactly what a data-plane defect looks like. It is exactly 1.0 on every step.vlm_grpo-nemotron-omni-30ba3b-clevr-1n8g-automodel-ep8.v1-tq_mooncake— 10/10 steps, 2N x 8G.Numbers below are from the
simplebackend (runfaxjtlfy); the wrapper now selectsmooncake_cpuso the two VLM nightlies cover one backend each, and a mooncake run of the same recipe is in flight.Wandb: https://wandb.ai/nvidia/nemorl-dataplane-zhiyul/runs/faxjtlfy
max(train/token_mult_prob_error)max(train/reward)token_mult_prob_errorcompares rollout logprobs againstprev_logprobs, both computed over the images, so it detects a wire round-trip that alters pixel data. Measured 1.0137–1.0155 across eight runs spanning the legacy (data_plane.enabled=false) path and three wire formats — a 0.002 spread, which is why the 1.02 bound is loose enough not to flake and tight enough to catch corruption.probs_ratiois not gated here: this recipe takes 16 inner steps per rollout, so the ratio measures policy drift rather than data fidelity, and its max ranges 2.3–29.2 across runs of identical code including the no-data-plane control.The omni wrapper also runs its gate out-of-band-proof: vLLM's teardown can exit nonzero after training completes, which used to skip
check_metricsentirely. It now keeps the driver's status, regeneratesmetrics.jsonfrom the tensorboard logs when the teardown skipped it, fails the run if it stopped short ofMAX_STEPS, and only then evaluates the gate.Earlier mooncake run (Qwen3.5, pre-refactor wire): https://wandb.ai/nvidia/nemorl-dataplane-zhiyul/runs/qtn9sl4u
Why the omni wrapper sets
cluster.num_nodes: 2The base recipe is 1n8g with
expert_parallel_size: 8, which makesdp = world/(tp*cp*ep) = 8/8 = 1— FSDP2 then shards nothing along the data-parallel axis and every rank holds a full copy of the dense params, grads and optimizer state, 72.2 GiB of a 79 GiB card. That fit only just, and stopped fitting when main picked up Automodel r0.6.0 (c158373): the base recipe now dies on a 306 MiB allocation inDTensorPolicyWorkerV2.train_presharded()at step 1.This is upstream of the data plane, not caused by it — the same OOM reproduces with
data_plane.enabled=false, and loweringgpu_memory_utilization(0.5 / 0.4 / 0.35) does not help because vLLM's post-sleep residual is a fixed 4.12 GiB.dp=2halves the dense side and the recipe trains again; the experts stay sharded 8 ways either way. The file keeps its1n8gname becausecommon-tq.envderivesBASE_RECIPEby stripping the-tq_simplesuffix, so renaming the wrapper would orphan it from the recipe it delegates to.Review fixes
Addresses the four review comments plus findings from a follow-up review:
train_from_metafetched only the static text-onlyDP_TRAIN_FIELDSwhile the logprob dispatch shipped the multimodal columns. Both now use_present_multimodal_fields(meta).truncate_tensorsnarrowed wire-form multimodal tensors on dim 1 under dynamic batching; the AutoModel backend'scheck_sequence_dimneeded the same skip as megatron'sget_and_validate_seqlen.imgs_sizesregistered (plusnum_frames, its coupled partner).grpo_train_syncacceptsprocessorfor launcher signature parity.pixel_dtype, matching the legacy analogs —pixel_valueswere crossing the wire in fp32 where legacy shipped bf16.codec.materializeno longer rectangularizes packed multimodal fields, which removed the<key>__lengthscompanion per modality, the derivedPROMOTE_1D_FIELDSunion, and theLENGTHS_SUFFIXskips in both seq-dim validators andtruncate_tensors.to_wireflattens each segment to 1-D, so rows vary only in dim 0 andtorch.jaggedaccepts ragged trailing dims; the batch-max padding is gone from the wire and from TQ storage, and no pad target is transported.